feat: AST-based command unfurling for secure mode#21
Merged
Conversation
Replaces the byte-scanning validator (metacharacter blocklist + strings.Fields) with a shell-AST parser (mvdan.cc/sh/v3). This is an early-reject layer, not a security boundary — OS sandboxing remains the real containment. - unfurl.go: commandUnfurler parses each command and accepts only a single, fully-literal simple command (default-deny): no pipelines, lists, subshells, control flow, redirections, background, inline assignments, command/parameter/ arithmetic expansion, brace expansion or globs. It returns the resolved argv. - argpolicy.go: per-tool argument policies (git, find, tar) that strip the GTFOBins escape hatches structural validation can't see (git -c alias=!, find -exec, tar --checkpoint-action, ...). Interpreters with no governing policy are hard-denied even when allowlisted (was a warning). - security.go / executor.go: both consume the same unfurler, removing the previous double-parse (strings.Fields in two places was a parser-differential). - Parsers are pooled with sync.Pool: syntax.Parser is reusable but not concurrency-safe, so this keeps allocations low while staying race-free. Wins: quoted metacharacters are correctly treated as inert literals (echo ';' now works), and the whole obfuscation/substitution bypass class is rejected structurally rather than by an enumerated blocklist. Closes #19.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the phase-2 design from #19. Replaces secure mode's byte-scanning validator with a shell-AST parser.
What changed
unfurl.go—commandUnfurlerparses each command withmvdan.cc/sh/v3/syntaxand accepts only a single, fully-literal simple command (default-deny): no pipelines, lists, subshells, control flow, redirection, background, inlineFOO=bar, command/parameter/arithmetic expansion, brace expansion or globs. Returns the resolvedargv.argpolicy.go— per-tool argument policies (git,find,tar) that strip GTFOBins escape hatches structural validation can't see (git -c alias=!…,find -exec,tar --checkpoint-action, …). Interpreters with no governing policy are hard-denied even when allowlisted (previously only a warning).security.go/executor.go— both now consume the same unfurler, removing the previous double-parse (strings.Fieldsran in two places — a latent parser-differential). The dead metacharacter helpers are gone.syntax.Parseris reusable but not concurrency-safe, so parsers are pooled withsync.Pool: low allocations, race-free (the borrowed parser is returned only after the argv copies are built).Wins
echo ';'→ argv["echo", ";"](was a false reject).git -c alias.x=!cmdnow caught by the git policy;bash/pythonhard-denied if allowlisted.Limits (unchanged, stated honestly)
Runtime expansion,
evaland Rice's theorem make full malicious-intent detection undecidable. This over-rejects (default-deny) to make safe cases ergonomic and obvious-unsafe cases fail fast.Test
go build ./... && go vet ./... && go test -race ./...— green on go1.26.4. Newunfurl_test.go(accept/reject corpus incl. quoting, expansion, glob, control flow, ANSI-C) andargpolicy_test.go(per-tool); existing bypass corpus stays rejected; parallel subtests exercise the pooled parser under-race.Closes #19.